Skip to content

[FreeBSD] support FreeBSD #861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
May 23, 2025
Merged

[FreeBSD] support FreeBSD #861

merged 8 commits into from
May 23, 2025

Conversation

michael-yuji
Copy link
Member

This patch adds FreeBSD support to libdispatch.

This patch is partly based on #561 with bug fixes and uses the pipe based mechanism based on #559 to bridge with CFRunLoop, since part of the CFRunLoop changes already landed in CoreFoundation.

In addition to above changes, also implements workqueue and limited DispatchSource support.

@michael-yuji michael-yuji mentioned this pull request Mar 17, 2025
Open
compnerd
compnerd reviewed Mar 17, 2025
View reviewed changes
@jakepetroules jakepetroules moved this from Done to In Progress in Swift on FreeBSD Mar 18, 2025
@3405691582 3405691582 mentioned this pull request Mar 30, 2025
Merged
etcwilde
etcwilde reviewed May 22, 2025
View reviewed changes
Copy link
Contributor

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few places that look like it's duplicating the __unix__ case. Otherwise I think this looks pretty good.

@etcwilde
Copy link
Contributor

@swift-ci please test

etcwilde
etcwilde approved these changes May 22, 2025
View reviewed changes
Copy link
Contributor

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CodingCarpincho
CodingCarpincho approved these changes May 22, 2025
View reviewed changes
@etcwilde etcwilde requested a review from compnerd May 22, 2025 20:52
@michael-yuji
Copy link
Member Author

@swift-ci please test

compnerd
compnerd reviewed May 22, 2025
View reviewed changes
src/event/workqueue.c
int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID | KERN_PROC_INC_THREAD, (int)getpid()};

// get size we need
if (sysctl(mib, 4, NULL, &size, NULL, 0) < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use ARRAY_SIZEOF for the second parameter.

#define ARRAY_SIZEOF(array) (sizeof((array))/sizeof(*(array)))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think it's a good idea to introduce a macro, especially just for one place.

Although it makes it more readable, but I think it is reasonable to expect the next person who need to change this line are already familiar with the correct usage of sysctl

src/swift/Source.swift
@@ -124,8 +124,17 @@ extension DispatchSource {
public static let exit = ProcessEvent(rawValue: 0x80000000)
public static let fork = ProcessEvent(rawValue: 0x40000000)
public static let exec = ProcessEvent(rawValue: 0x20000000)
#if os(FreeBSD)
public static let track = ProcessEvent(rawValue: 0x00000001)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the different name? Should we not be using the name names to maintain source consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because NOTE_TRACK is only available on FreeBSD but not Darwin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, having an extended FreeBSD only option is fine, but this does remove the signal note doesn't it? I take it that NOTE_TRACK is meant to replace that? If so, why not keep the name incongruous with the underlying note to allow easier porting?

Copy link
Member Author

@michael-yuji michael-yuji May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've add more context in my previous reply. NOTE_SIGNAL on Darwin and NOTE_TRACK on FreeBSD are not interchangeable and each are platform specific.

NOTE_SIGNAL on Darwin allow users to monitor if the process was sent a signal,

on the other hand, NOTE_TRACK on FreeBSD cause kqueue to follow the target process across fork and automatically register events with same monitor. For example, NOTE_TRACK | NOTE_EXIT on a pid cause exit event of the tracing process, and all its decedents to be reported.

@etcwilde
Copy link
Contributor

etcwilde commented May 23, 2025 •
edited
Loading

It's not in this PR, but I just came across an additional build failure in the tests:

swift-corelibs-libdispatch/tests/dispatch_workqueue.c

Lines 33 to 47 in beb137b

activecpu(void)
{
uint32_t activecpu;
#if defined(__linux__) || defined(__OpenBSD__)
activecpu = (uint32_t)sysconf(_SC_NPROCESSORS_ONLN);
#elif defined(_WIN32)
SYSTEM_INFO si;
GetSystemInfo(&si);
activecpu = si.dwNumberOfProcessors;
#else
size_t s = sizeof(activecpu);
sysctlbyname("hw.activecpu", &activecpu, &s, NULL, 0);
#endif
return activecpu;
}

I think we need to adjust the macro to #if defined(__linux__) || defined(__OpenBSD__) || defined(__FreeBSD__).
I know that FreeBSD has sysctlbyname, but hw.activecpu returns the physical core count, while dwNumberOfProcessors is the logical core count, so sysconf(_SC_NPROCESSORS_ONLN) seems to be more consistent. Up to you if you want to add that in this PR or not.

edit: Realizing I didn't add the actual failure.
The error is that we're hitting the sysctlbyname call without including sys/sysctl.h.

@etcwilde
Copy link
Contributor

@swift-ci please test

@etcwilde
Copy link
Contributor

@swift-ci please test Windows

@michael-yuji michael-yuji merged commit 089b658 into swiftlang:main May 23, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Swift on FreeBSD May 23, 2025
@michael-yuji
Copy link
Member Author

It's not in this PR, but I just came across an additional build failure in the tests:

swift-corelibs-libdispatch/tests/dispatch_workqueue.c

Lines 33 to 47 in beb137b

activecpu(void)
{
uint32_t activecpu;
#if defined(__linux__) || defined(__OpenBSD__)
activecpu = (uint32_t)sysconf(_SC_NPROCESSORS_ONLN);
#elif defined(_WIN32)
SYSTEM_INFO si;
GetSystemInfo(&si);
activecpu = si.dwNumberOfProcessors;
#else
size_t s = sizeof(activecpu);
sysctlbyname("hw.activecpu", &activecpu, &s, NULL, 0);
#endif
return activecpu;
}

I think we need to adjust the macro to #if defined(__linux__) || defined(__OpenBSD__) || defined(__FreeBSD__). I know that FreeBSD has sysctlbyname, but hw.activecpu returns the physical core count, while dwNumberOfProcessors is the logical core count, so sysconf(_SC_NPROCESSORS_ONLN) seems to be more consistent. Up to you if you want to add that in this PR or not.

edit: Realizing I didn't add the actual failure. The error is that we're hitting the sysctlbyname call without including sys/sysctl.h.

Will be addressing this in the next PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

@etcwilde etcwilde etcwilde approved these changes

@CodingCarpincho CodingCarpincho CodingCarpincho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Swift on FreeBSD
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michael-yuji @etcwilde @compnerd @CodingCarpincho